Skip to content

Promote dev to main: dedup reads REDIS_URL via routerConfig (envScrub regression fix)#1251

Merged
zbigniewsobiecki merged 1 commit intomainfrom
dev
May 1, 2026
Merged

Promote dev to main: dedup reads REDIS_URL via routerConfig (envScrub regression fix)#1251
zbigniewsobiecki merged 1 commit intomainfrom
dev

Conversation

@zbigniewsobiecki
Copy link
Copy Markdown
Member

Promotes 1 commit from `dev`:

What

Live regression fix: PR #1248's Redis-backed dedup throws inside cascade-worker processes because `envScrub.ts` strips `REDIS_URL` from `process.env` early in worker startup, and the dedup module reads it lazily afterward. Switch to `routerConfig.redisUrl` (captured at module-load) which survives the scrub the same way the DB pool's cached connection string does.

Reviewed and approved on #1250 by @nhopeatall. CI green on dev (3/3 expected).

Risk

Low. Two-line source change + one regression-pin test. Pure read-source switch — no semantic change to the dedup logic.

🤖 Generated with Claude Code

…crub (#1250)

PR #1248's Redis-backed dedup throws inside cascade-worker processes:

  ERROR Review-dispatch dedup Redis call failed — failing closed
    error: 'Error: REDIS_URL is required for review-dispatch dedup'

Confirmed live at 2026-05-01T17:25:44 in worker
`cascade-worker-coalesce_ucho_MNG-461_*`. Sentry tag: `review_dedup_redis_down`.

Root cause: `src/utils/envScrub.ts:13-18` lists REDIS_URL in
`SENSITIVE_ENV_KEYS`. Worker-entry calls `scrubSensitiveEnv()` early in
startup, which does `delete process.env.REDIS_URL` to keep infra secrets
out of agent-spawned subprocesses. The dedup module reads
`process.env.REDIS_URL` lazily at first dispatch — well after the scrub —
so it sees `undefined` and throws. The fail-closed handler returns false,
and the post-completion-hook silently logs `Skipping post-completion
review: already dispatched` (which looks identical to the working dedup's
"correctly rejected duplicate" log, hence the bug evaded the prod canary).

Why it didn't bite immediately: the post-completion-hook short-circuits
when CI isn't all passing at impl exit
(`agent-execution.ts:269-275`), so the bug only triggers on the narrow
timing where CI is fully green BEFORE impl finishes. Small fraction of
runs.

Why router is unaffected: cascade-router never calls scrubSensitiveEnv.
PR #196's review (verified 1-run earlier) was dispatched from the router
via check-suite-success — that path was never broken.

Fix: read the URL via `routerConfig.redisUrl` instead of
`process.env.REDIS_URL`. routerConfig is captured at module-load time in
`src/router/config.ts:137-138`, well before scrubSensitiveEnv runs. The
cached value survives the scrub the same way the DB pool's cached
connection string does — exactly the pattern envScrub.ts's docstring
describes.

Two-line source change + new regression-pin test that claims, deletes
process.env.REDIS_URL, and claims again — both must succeed (second
deduped) without throwing `REDIS_URL is required`.

Verification: vitest 2497/2497, typecheck clean, lint clean.

Out of scope (follow-up): `src/queue/cancel.ts` has the same
`process.env.REDIS_URL` lazy pattern but is only called from cascade-router
and cascade-dashboard (neither scrubs env), so it doesn't bite today.
Apply the same routerConfig swap for symmetry in a separate PR.

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@codecov
Copy link
Copy Markdown

codecov Bot commented May 1, 2026

Codecov Report

❌ Patch coverage is 66.66667% with 1 line in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/triggers/github/review-dispatch-dedup.ts 66.66% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

@zbigniewsobiecki zbigniewsobiecki merged commit 1f61983 into main May 1, 2026
14 of 15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant